Skip to content

refactor(vortex-geo): generalize scalar function execution - #9215

Open
HarukiMoriarty wants to merge 1 commit into
developfrom
nemo/geo-scalar-execution
Open

refactor(vortex-geo): generalize scalar function execution#9215
HarukiMoriarty wants to merge 1 commit into
developfrom
nemo/geo-scalar-execution

Conversation

@HarukiMoriarty

@HarukiMoriarty HarukiMoriarty commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

Native geometry scalar functions need consistent handling for constant operands, pairwise columns, null propagation, and all-null batches. Keeping those responsibilities in each function makes new unary and binary kernels harder to implement correctly.

What changes are included in this PR?

  • Split shared geometry execution into unary, binary, and geo-types adapter modules.
  • Represent constant and column operands explicitly in shared execution state.
  • Centralize strict null propagation and all-null short-circuiting.
  • Preserve constant outputs when all operands are constant.
  • Migrate Envelope, Contains, Distance, and Intersects to the shared execution layer.
  • Keep bounding-box prechecks scoped to one-constant binary predicate paths.
  • Move native Rect array assembly into the Rect extension module.

What APIs are changed? Are there any user-facing changes?

This is an internal vortex-geo refactor. Existing scalar function semantics remain unchanged.

@HarukiMoriarty HarukiMoriarty added the changelog/skip Do not list PR in the changelog label Aug 5, 2026
@HarukiMoriarty HarukiMoriarty mentioned this pull request Aug 5, 2026
Open
connortsui20
connortsui20 approved these changes Aug 5, 2026
View reviewed changes

@connortsui20 connortsui20 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gatesn the RowFn work that I am working on (#9128) will supersede this, but I think this is fine to merge?

@codspeed-hq

codspeed-hq Bot commented Aug 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will degrade performance by 20.78%

❌ 3 regressed benchmarks
✅ 1892 untouched benchmarks
⏩ 43 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation decode_varbin[(1000, 2)] 62.1 µs 78.6 µs -20.97%
❌ Simulation point_random_nulls 72.9 µs 92.1 µs -20.81%
❌ Simulation point_mixed_validity 72.4 µs 91.2 µs -20.55%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

Comparing nemo/geo-scalar-execution (09f16ff) with develop (c3639a2)2

Open in CodSpeed

Footnotes

  1. 43 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on develop (9031788) during the generation of this report, so c3639a2 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@HarukiMoriarty
HarukiMoriarty force-pushed the nemo/geo-scalar-execution branch from 5a42d1b to 751b43b Compare August 5, 2026 19:17
@HarukiMoriarty
refactor(vortex-geo): generalize scalar execution
09f16ff 
Signed-off-by: Nemo Yu <zyu379@wisc.edu>
@HarukiMoriarty
HarukiMoriarty force-pushed the nemo/geo-scalar-execution branch from 751b43b to 09f16ff Compare August 5, 2026 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@connortsui20 connortsui20 connortsui20 approved these changes

Assignees

No one assigned

Labels

changelog/skip Do not list PR in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@HarukiMoriarty @connortsui20